Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Partial implementation of certmanager for integration sink #8481

Merged
merged 22 commits into from
Feb 19, 2025
Merged

Partial implementation of certmanager for integration sink #8481

merged 22 commits into from
Feb 19, 2025

Conversation

matzew
Copy link
Member

@matzew matzew commented Feb 19, 2025

Proposed Changes

  • adding cert manager dependency and generating code
  • disabled it for now - but sharing for other parts to be able to use it

Next steps (or PRs)

  • isolate the "certmanager" library in a common place (so that event transformer can also "easily" use it
  • bump cert manager version and use their client set (like serving does)
  • Enable cert manager for integration sink and have dynamic knob for feature toggle (e.g. on/off)

matzew added 19 commits February 18, 2025 17:40
@matzew
Adding cert-mananger bits
ffc8377 
Basics for Cert-manager

Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Adding create cert manifest function
ba26ba5 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Use the make cert
3c6a30a 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Reconcile Certificate manifest and add RBAC
2df990c 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Add secret filter to controller
f6fa1df 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Look up secrets/certs for sink
cf0574d 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Remove old/wrong Certificate manifest and reference
0d49024 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Fix controller compile
c12dea3 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Formatting
6af8b8a 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
change the reconcile of cert manifest, and run it only w/ some form o…
31bf340 
…f TLS

Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
using filtered informer
3b6e31d 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Adding Rekt test for IntegrationSink TLS support
5643b5e 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
adding a bit of https port for deployment/service of the IntegrationSink
b166f10 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Mounting secrets and setting quarkus env vars for TLS support
d9b646e 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Adding flags for different TLS levels resulting in diffeerent quarkus…
a2909da 
… env vars

Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Remove bad comment
5203878 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Adding label
5a235c1 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Update codegen
34b8cd2 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
Disable tmp. the cert manager feature
7400edd 
Signed-off-by: Matthias Wessendorf <[email protected]>
@knative-prow knative-prow bot added the area/test-and-release Test infrastructure, tests or release label Feb 19, 2025
@knative-prow Knative Prow
Copy link

knative-prow bot commented Feb 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot requested review from mgencur and pierDipi February 19, 2025 08:51
@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 19, 2025
@matzew
Fixing header/year
3059b60 
Signed-off-by: Matthias Wessendorf <[email protected]>
@matzew
More test tweaking
05de212 
Signed-off-by: Matthias Wessendorf <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Feb 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 42.94872% with 89 lines in your changes missing coverage. Please review.

Project coverage is 62.64%. Comparing base (9a7c6d5) to head (5a0c1de).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...conciler/integration/sink/resources/certificate.go 0.00% 44 Missing ⚠️
pkg/reconciler/integration/sink/integrationsink.go 31.42% 24 Missing ⚠️
...iler/integration/sink/resources/container_image.go 87.09% 7 Missing and 1 partial ⚠️
pkg/reconciler/integration/sink/controller.go 0.00% 6 Missing ⚠️
cmd/controller/main.go 0.00% 5 Missing ⚠️
pkg/reconciler/integration/sink/resources/names.go 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8481      +/-   ##
==========================================
- Coverage   62.77%   62.64%   -0.14%     
==========================================
  Files         395      397       +2     
  Lines       24108    24241     +133     
==========================================
+ Hits        15135    15185      +50     
- Misses       8166     8247      +81     
- Partials      807      809       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

1 similar comment
@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/retest

@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

2 similar comments
@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

pierDipi
pierDipi reviewed Feb 19, 2025
View reviewed changes
cmd/controller/main.go Outdated
Comment on lines 75 to 77
for _, inf := range []injection.InformerInjector{challenge.WithInformer, v1certificate.WithInformer, certificaterequest.WithInformer, clusterissuer.WithInformer, issuer.WithInformer} {
injection.Default.RegisterInformer(inf)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cert-manager is not installed, I think this logs errors and gets the controller stack on starting? Did you try that?

Also I don't think we need the clusterissuer, issuer, challenge, or certificaterequest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - forgot to comment that out

@pierDipi
Copy link
Member

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2025
@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

1 similar comment
@matzew
Copy link
Member Author

matzew commented Feb 19, 2025

/test unit-tests

@knative-prow knative-prow bot merged commit 732ec62 into knative:main Feb 19, 2025
30 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@mgencur mgencur Awaiting requested review from mgencur

Assignees

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matzew @pierDipi